Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMoved a flaky invalid-UTF8 test into its own t.Run and changed its control flow; added a syncSpanProcessor to route span export errors to an instance-local handler and replaced memory exporter registration to use that processor for per-instance error handling. No public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/telemetry/attribute_processor_test.go`:
- Around line 198-201: Restore the eventual wait around the post-request
assertion to avoid races: after calling
xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{...}) wrap the check that
res.Response.StatusCode equals 200 in require.Eventually (referencing
require.Eventually and res.Response.StatusCode) so the test polls until the
export/log side-effect has been observed instead of asserting immediately;
ensure the timeout and polling interval are reasonable for the async export/log
delivery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: decaa42c-56f1-4ff5-8296-ae63562876d1
📒 Files selected for processing (1)
router-tests/telemetry/attribute_processor_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2597 +/- ##
==========================================
- Coverage 64.11% 61.61% -2.51%
==========================================
Files 301 245 -56
Lines 42642 25770 -16872
Branches 4538 0 -4538
==========================================
- Hits 27342 15877 -11465
+ Misses 15279 8508 -6771
- Partials 21 1385 +1364
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router-tests/telemetry/attribute_processor_test.go (1)
240-250: Stabilize this flaky assertion by separating trigger from polling.Line 241 currently sends a new request on every
Eventuallypoll, and line 250 depends onlogs[0]ordering. This can still be nondeterministic. Send one request, then poll only for the expected message across all matching log entries.Suggested refactor
}, func(t *testing.T, xEnv *testenv.Environment) { - require.Eventually(t, func() bool { - xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ - Query: `query { employees { id } }`, - }) - logs := xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() - return len(logs) > 0 - }, 10*time.Second, 500*time.Millisecond) - - logs := xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() - require.NotEmpty(t, logs) - require.Equal(t, "otel error: traces export: string field contains invalid UTF-8: Enable 'telemetry.tracing.sanitize_utf8.enabled' in your config to sanitize invalid UTF-8 attributes.", logs[0].Message) + expected := "otel error: traces export: string field contains invalid UTF-8: Enable 'telemetry.tracing.sanitize_utf8.enabled' in your config to sanitize invalid UTF-8 attributes." + + xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ + Query: `query { employees { id } }`, + }) + + require.Eventually(t, func() bool { + logs := xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() + for _, entry := range logs { + if entry.Message == expected { + return true + } + } + return false + }, 10*time.Second, 500*time.Millisecond) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/telemetry/attribute_processor_test.go` around lines 240 - 250, The test currently re-sends the GraphQL request on every Eventually poll and then asserts logs[0], which causes flakiness; instead call xEnv.MakeGraphQLRequestOK(...) once before polling, then use require.Eventually to repeatedly read xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() and check that any entry's Message equals the expected string (or that the slice contains the expected message), finally assert the logs slice is not empty and contains the expected message — update references in the test to xEnv.MakeGraphQLRequestOK, xEnv.Observer().FilterMessageSnippet(...).All(), and the expected message string so ordering of logs[0] is not relied upon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router-tests/telemetry/attribute_processor_test.go`:
- Around line 240-250: The test currently re-sends the GraphQL request on every
Eventually poll and then asserts logs[0], which causes flakiness; instead call
xEnv.MakeGraphQLRequestOK(...) once before polling, then use require.Eventually
to repeatedly read xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All()
and check that any entry's Message equals the expected string (or that the slice
contains the expected message), finally assert the logs slice is not empty and
contains the expected message — update references in the test to
xEnv.MakeGraphQLRequestOK, xEnv.Observer().FilterMessageSnippet(...).All(), and
the expected message string so ordering of logs[0] is not relied upon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5bde0d8-c13b-421e-8f3a-a4053ecf7eb2
📒 Files selected for processing (1)
router-tests/telemetry/attribute_processor_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/telemetry/attribute_processor_test.go`:
- Around line 241-251: The test currently calls xEnv.MakeGraphQLRequestOK inside
require.Eventually which replays the request each poll and produces multiple
logs; move the single call to xEnv.MakeGraphQLRequestOK out of the
require.Eventually so Eventually only polls for the log presence. Inside the
polling closure use xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All()
to detect that any matching log has appeared (no request inside), and when
asserting the final log verify by searching the returned logs for an entry whose
Message equals the expected string instead of assuming logs[0] (use the Message
field comparison against "otel error: traces export: string field contains
invalid UTF-8: Enable 'telemetry.tracing.sanitize_utf8.enabled' in your config
to sanitize invalid UTF-8 attributes.").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37f0cb0a-86dc-446e-92bf-50e9bd8eed66
📒 Files selected for processing (1)
router-tests/telemetry/attribute_processor_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
router-tests/telemetry/attribute_processor_test.go (1)
241-248:⚠️ Potential issue | 🟠 MajorAvoid order-dependent assertion on
logs[0]in a flaky-test fix path.Line 248 assumes deterministic ordering of matching logs. If multiple
sanitize_utf8logs exist, this can still fail intermittently.💡 Suggested stabilization
- require.Eventually(t, func() bool { - logs := xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() - return len(logs) > 0 - }, 10*time.Second, 500*time.Millisecond) - - logs := xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() - require.NotEmpty(t, logs) - require.Equal(t, "otel error: traces export: string field contains invalid UTF-8: Enable 'telemetry.tracing.sanitize_utf8.enabled' in your config to sanitize invalid UTF-8 attributes.", logs[0].Message) + expected := "otel error: traces export: string field contains invalid UTF-8: Enable 'telemetry.tracing.sanitize_utf8.enabled' in your config to sanitize invalid UTF-8 attributes." + require.Eventually(t, func() bool { + logs := xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() + for _, log := range logs { + if log.Message == expected { + return true + } + } + return false + }, 10*time.Second, 500*time.Millisecond)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/telemetry/attribute_processor_test.go` around lines 241 - 248, The test is brittle because it asserts logs[0].Message equals the expected string which fails if multiple matching "sanitize_utf8" entries appear; update the assertion in attribute_processor_test.go to be order-independent by checking that at least one entry in xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() has Message equal to the expected text (or assert the slice of messages contains the expected string) instead of directly indexing logs[0]; locate the require.NotEmpty(t, logs) and replace the require.Equal on logs[0].Message with a search/contains assertion over the messages returned by FilterMessageSnippet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/telemetry/attribute_processor_test.go`:
- Around line 237-240: The test only checks that MakeGraphQLRequest returned no
error (require.NoError) but ignores the actual HTTP response, allowing non-200
responses to pass; capture the response value returned by
xEnv.MakeGraphQLRequest (e.g., resp, err := xEnv.MakeGraphQLRequest(...)) and
add an assertion that the response status is 200 (use require.Equal or
require.EqualValues against resp.StatusCode or resp.Status depending on the
response type) immediately after require.NoError to ensure non-200 responses
fail the test.
---
Duplicate comments:
In `@router-tests/telemetry/attribute_processor_test.go`:
- Around line 241-248: The test is brittle because it asserts logs[0].Message
equals the expected string which fails if multiple matching "sanitize_utf8"
entries appear; update the assertion in attribute_processor_test.go to be
order-independent by checking that at least one entry in
xEnv.Observer().FilterMessageSnippet("sanitize_utf8").All() has Message equal to
the expected text (or assert the slice of messages contains the expected string)
instead of directly indexing logs[0]; locate the require.NotEmpty(t, logs) and
replace the require.Equal on logs[0].Message with a search/contains assertion
over the messages returned by FilterMessageSnippet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39857a05-b4ac-433a-8916-6250f03dee87
📒 Files selected for processing (3)
router-tests/telemetry/attribute_processor_test.gorouter/pkg/trace/meter.gorouter/pkg/trace/syncspan_test_exporter.go
|
Thank you. It was already addressed in 24ebf0a |
This PR fixes a flaky test because parallel tests overrides the error handler, which means that the utf8 warning message test's error handler with the particular log observer instance might be overridden
Summary by CodeRabbit
Checklist